Support multiple glob patterns in sub-config matches#3516
Conversation
Change SubConfig.matches from a single Glob to Vec<Glob> and add a custom serde deserializer that accepts either a single glob string or an array of glob strings for backward compatibility. Update matching logic (rewrite_with_path_to_config, matches_path) and readers/migrations to produce Vec<Glob>. Update tests, migration code, JSON schema, TOML examples and docs to reflect that `matches` can be a string or list of strings and to exercise multiple-pattern behavior.
|
Hi @kavix! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Hey Kavix, thank you for signing the Meta CLA license and welcome aboard to the team! Let me review this PR and get back to you. This is a useful feature that will reduce config duplication. Tagging @kinto0 for config ownership review. Put up some comments on the follow-up, once these are addressed, this should be good to land! |
| "matches": { | ||
| "description": "A filesystem glob pattern detailing which files the config applies to.", | ||
| "type": "string" | ||
| "description": "One or more filesystem glob patterns detailing which files the config applies to.", |
There was a problem hiding this comment.
This file has a lot of formatting changes unrelated to the matches field (enum values expanded to multi-line, arrays expanded, etc.). Could you regenerate using the standard tooling to minimize diff noise? The actual schema change for matches (allowing string or string[]) is correct.
There was a problem hiding this comment.
so do the .toml files underneath
NathanTempest
left a comment
NathanTempest
left a comment
There was a problem hiding this comment.
The implementation is solid, the serde deserializer correctly handles both string and array inputs, and the migration code properly emits arrays. I especially like that the mypy migration now consolidates multiple modules into a single sub-config with multiple globs.
Before we merge, a few things to address:
Please verify the error message formatting in crates/pyrefly_config/src/config.rs. The change from sub_config.matches (a Glob) to computing a matches string and using it in the format string looks correct, but please confirm this compiles and consider adding a test that triggers the "Extra keys found in sub config" warning to ensure the formatting works as expected.
Schema file formatting. The schemas/pyrefly.json diff includes a lot of whitespace changes unrelated to the matches field (enum values expanded to multi-line, arrays expanded, etc.). Could you regenerate this file using the project's standard schema generation tooling to avoid formatting drift? If the schema is hand-maintained, please keep the formatting consistent with the existing file.
| }; | ||
| Ok(SubConfig { | ||
| matches: Glob::new(self.root)?, | ||
| matches: vec![Glob::new(self.root)?], |
There was a problem hiding this comment.
did pyright allow both single glob and vec of glob? can we have tests for what happens in both cases?
|
|
||
| - a `matches` key, with a [Filesystem Glob](#filesystem-globbing) detailing which files the config | ||
| applies to. | ||
| - a `matches` key, with a [Filesystem Glob](#filesystem-globbing) or list of globs detailing which |
There was a problem hiding this comment.
if it should be both a single glob or a list, do we have test coverage for both?
3 participants
This change lets [[sub-config]] blocks target more than one path pattern by accepting a list of glob strings in matches. The matching logic now applies a sub-config when any pattern matches, which makes large config migrations much less repetitive.
It also keeps existing single-string configs working for compatibility, updates MyPy and Pyright migration paths to emit the new shape, and refreshes the schema, examples, and docs to reflect the new syntax.